Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix underflow in fee estimation in the presence of withdrawal and max amount #2086

Merged

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Aug 27, 2020

Issue Number

#2062

Overview

10ab550 out of paranoia, add an invariant to make transactions never leave fees beyond a certain amount.
3d4469c pass down withdrawal value to 'handleCannotCoverFee' to prevent underflow in fee calculation.
f6e74dc add extra scenario covering fee estimation edge-case in the presence of withdrawal

TODO: I'd like to add one property or scenario to exercise the invariant and shows that it operates as expected.

Comments

@KtorZ KtorZ requested review from Anviking and piotr-iohk August 27, 2020 18:51
@KtorZ KtorZ self-assigned this Aug 27, 2020
lib/core/src/Cardano/Wallet/Primitive/Fee.hs Outdated Show resolved Hide resolved
lib/core/src/Cardano/Wallet/Primitive/Fee.hs Outdated Show resolved Hide resolved
, expectField (#estimatedMax . #getQuantity) (.<= oneAda)
]

it "TRANS_ESTIMATE_03b - we see result when we can't cover fee (with withdrawal)" $ \ctx -> do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it "TRANS_ESTIMATE_03b - we see result when we can't cover fee (with withdrawal)" $ \ctx -> do
it "TRANS_ESTIMATE_03b - we see result when we can't cover fee \
\(with withdrawal)" $ \ctx -> do

lib/core/src/Cardano/Wallet.hs Show resolved Hide resolved
lib/core/src/Cardano/Wallet/Primitive/Fee.hs Show resolved Hide resolved
Copy link
Contributor

@piotr-iohk piotr-iohk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm from black-box functional perspective.

I have tested on MC4 with self and mnemonic sentence withdrawal on a wallet with rewards and fee amt is OK. The same fee amt is then applied when sending tx.

…of withdrawal

  This currently fails, 'as expected', next commit will fix the issue.
…es beyond a certain amount.

  This should _not_ be the case, but the contrary would be disastrous for the user. I was getting a little paranoid about this recently,
  so I think having the wallet crash loudly in such case is a good and sane idea.
@KtorZ KtorZ force-pushed the KtorZ/2062/fix-underflow-fee-estimation-withdrawal branch from 10ab550 to f0e7ae2 Compare September 2, 2020 11:31
@KtorZ
Copy link
Member Author

KtorZ commented Sep 2, 2020

bors r+

@KtorZ KtorZ added the RESOLVING ISSUE Mark a PR as resolving issues, for auto-generated CHANGELOG label Sep 2, 2020
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 2, 2020

Build succeeded

@iohk-bors iohk-bors bot merged commit db981dd into master Sep 2, 2020
@iohk-bors iohk-bors bot deleted the KtorZ/2062/fix-underflow-fee-estimation-withdrawal branch September 2, 2020 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RESOLVING ISSUE Mark a PR as resolving issues, for auto-generated CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants